-
-
Notifications
You must be signed in to change notification settings - Fork 51
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Modernize bgcolor #294
base: master
Are you sure you want to change the base?
Modernize bgcolor #294
Conversation
@vibraphone why the css contains style/classes for other components? model.container.querySelector('.bg-color-container').style.backgroundColor = model.color; What was the API change that you wanted? ;-) |
@jourdain d3 is in there because I'm comfy with it. I will clean up the CSS. The API change I really wanted was what's mentioned in the commit: adopting the |
@jourdain d3 is in there because I'm comfy with it. I will clean up the CSS. The API change I really wanted was what's mentioned in the commit: adopting the |
6a9580a
to
7c10355
Compare
@jourdain I've removed d3 and cleaned up the CSS. |
return; | ||
} | ||
|
||
model.clientRect = model.container.getBoundingClientRect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that method could remain empty...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like the method to be there (so that, for instance, it is easier to copy the component as a starting point for new components). Is it OK for the method to exist but do nothing? Or should I find something "demo-like" for it to do on resize to justify its existence?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes the method should exist, but do nothing. You can put its content inside a comment so other component could use the actual content by uncommenting it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when using it as a template/starting-point.
LGTM otherwise |
const green = new BGColorComponent({ color:'green' }); | ||
const red = new BGColorComponent({ color:'red' }); | ||
const blue = new BGColorComponent({ color:'blue' }); | ||
const pink = new BGColorComponent({ color:'pink' }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this one use 'new' when the other example use 'newInstance'?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch Aron, that should be newInstance() now...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, Aron. Should be fixed now.
7c10355
to
eea993a
Compare
@jourdain @aronhelser Thanks for the feedback. |
Thinking about it, you should have a destroy() method that call setContainer(null) and call the "parent" destroy. |
Now `BackgroundColor` provides a `newInstance()` method that takes publicAPI and model objects, making it a good minimal example. It also demonstrates how to import HTML fragments and CSS style from external files. See the `ToggleControl` and `Workbench` examples for usage.
eea993a
to
858038b
Compare
No description provided.